Fix phpstan/phpstan#14449: Another false positive#5438
Fix phpstan/phpstan#14449: Another false positive#5438phpstan-bot wants to merge 2 commits intophpstan:2.1.xfrom
Conversation
…t from ArrayDimFetch - When creating a reference from an ArrayDimFetch (e.g. $ref = &$arr['key']), set up IntertwinedVariableByReferenceWithExpr so writes through $ref propagate back to $arr - Added propagateRefTypeToArrayDimFetch() to handle constant-dim ArrayDimFetch targets using setOffsetValueType instead of HasOffsetValueType intersection which would produce never type for incompatible constant array shapes - New regression test in tests/PHPStan/Rules/Comparison/data/bug-14449.php
VincentLanglet
left a comment
There was a problem hiding this comment.
I don't see why hasConstantDimInChain check is needed.
Also is there a simpler way to implement propagateRefTypeToArrayDimFetch ?
Replace the two-pass approach (forward pass to collect intermediate types, backward pass to rebuild) with a single while loop that walks from leaf to root, getting each parent's type directly from scope. Also remove unused array_reverse import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's a summary of what was done: Changes made to address the review:
|
Summary
PHPStan incorrectly reported
array_key_exists()as always evaluating to false when checking an array that was modified through a by-reference variable assigned from anArrayDimFetchexpression (e.g.$ref = &$arr[$id]['data']). Writes through$refwere not propagated back to$arr, so in loop iterations the type of$arr[$id]['data']remainedarray{}even after keys were added via the reference.Changes
src/Analyser/ExprHandler/AssignHandler.php: When processingAssignRefwhere the right side is anArrayDimFetch, set up anIntertwinedVariableByReferenceWithExprso that writes to the reference variable propagate back to the source array.src/Analyser/MutatingScope.php: AddedpropagateRefTypeToArrayDimFetch()method that usessetOffsetValueType(replacement semantics) instead of the defaultassignExpressionpath which usesHasOffsetValueTypeintersection (narrowing semantics). The narrowing path fails for constant-dim ArrayDimFetch because intersectingarray{data: array{}}withHasOffsetValueType('data', array{id: int})producesnever. AddedhasConstantDimInChain()helper to detect when this alternative propagation is needed.tests/PHPStan/Rules/Comparison/data/bug-14449.php: Regression test reproducing the original issue.tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php: Test method for the regression test.Root cause
When
$resultData = &$results[$id]['data']was processed, PHPStan set up noIntertwinedVariableByReferenceWithExprfor this case (only Variable-to-Variable references were handled). This meant that$resultData['id'] = $idonly updated the type of$resultDatabut not$results[$id]['data']. During loop fixpoint analysis, the type of$results[$id]['data']converged toarray{}(from the explicit$results[$id]['data'] = []assignment) without accounting for modifications through the reference. Consequently,array_key_exists('id', $results[$id]['data'])was incorrectly reported as always false.The existing
assignExpressionmethod couldn't be used directly for propagation becausespecifyExpressionTypeusesHasOffsetValueTypeintersection for constant-key ArrayDimFetch, which producesneverwhen the existing value type is incompatible with the new value type (e.g.,array{}intersected withHasOffsetValueType('data', array{id: int})). The fix usessetOffsetValueTypeinstead, which replaces the offset value type rather than narrowing it.Test
Added
tests/PHPStan/Rules/Comparison/data/bug-14449.php— reproduces the exact code from the issue: a loop witharray_key_existschecks, by-reference assignment to a nested array element, and modification through the reference. The test expects no errors (the false positive is eliminated).Fixes phpstan/phpstan#14449